-
Notifications
You must be signed in to change notification settings - Fork 33
HTML documentation #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTML documentation #103
Conversation
This still requires some work to fix cross-linking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads quite well, thanks Aaron. I didn't try to build locally, did read the whole diff and things are structured well. I only had a few minor comments.
It may be worth adding a CI job to run on PRs, failing on Sphinx warnings/errors, and then to deploy to GitHub Pages on merges. Can also be done as a follow-u. Either way, LGTM to merge this.
docs/dev/implementation-notes.md
Outdated
|
||
|
||
## Avoiding Code Duplication | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section may want to add a mention of JAX and Dask, even to just mention that they exist. Or use "for example" when namespaces are explicitly listed
docs/dev/implementation-notes.md
Outdated
not strictly required by the standard. | ||
|
||
array-api-tests is run against all supported libraries are tested on CI | ||
(except for JAX, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "except for JAX, wrapping" phrasing is a little odd - rephrase?
I actually haven't finished here (hence the "draft" status), but thanks for taking a look. I would especially like your thoughts on this section https://github.com/data-apis/array-api-compat/pull/103/files#diff-ebbf0983decf5c6b5520f86b136672b0a044a778687a8813de4900f4f554d58eR7. Specifically, this paragraph, https://github.com/data-apis/array-api-compat/pull/103/files#diff-ebbf0983decf5c6b5520f86b136672b0a044a778687a8813de4900f4f554d58eR36-R40. I think it would be good to have a policy on what we will and won't include in this library. A lot of people have made suggestions for things that could go here which I personally feel would be out of scope (e.g., I'm hesitant on even #104, because it's outside the domain of the standard, and I'm really hesitant on suggestions to include things like additional helpers for functions that aren't in the standard). |
Actually we should probably add a separate "Scope" section to the main part of the docs to make this really clear to everyone. But I want to make sure that we are in alignment about what is and isn't in scope for this package. |
I agree with the way you have written it now. That matches my current understanding of what we're accepting into |
I've added a section to the main page explaining the scope of the package https://github.com/data-apis/array-api-compat/pull/103/files#diff-b4d68dc855d0f9476d3f2ee343853bd21bf82ea9960d0cf06661baa244439dd6R124 |
This is ready for review. You can see the rendered HTML version at https://data-apis.org/array-api-compat/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a section to the main page explaining the scope of the package
This looks quite good, it captures the intent and rationale well.
The main page is structured nicely too. I don't have substantial comments, it seems good to go. Nice work!
This reverts commit 99e663d.
Fixes #44